Skip to content

Moved the links to the sidebar #236#246

Merged
FedericoTartarini merged 23 commits intoCenterForTheBuiltEnvironment:developmentfrom
LeoLuosifen:yluo0664
Sep 26, 2025
Merged

Moved the links to the sidebar #236#246
FedericoTartarini merged 23 commits intoCenterForTheBuiltEnvironment:developmentfrom
LeoLuosifen:yluo0664

Conversation

@LeoLuosifen
Copy link
Contributor

Summary

This PR refactors the layout by moving the navigation links previously displayed in the top banner into a collapsible sidebar (dmc.Drawer) on the left side of the page. It also updates the end-to-end test file spec.cy.js to reflect the new layout and interaction behavior.

Changes

  • Replaced top tab navigation with a collapsible sidebar.
  • Used dash-mantine-components (dmc.NavLink, dmc.SegmentedControl, etc.) to build the new sidebar.
  • Moved tab navigation links (e.g., Select Weather File, Climate Summary, etc.) into the sidebar under “Pages Menu”.
  • Moved SI/IP and Global/Local Value Ranges controls into the sidebar under “Tools Menu”.
  • Ensured the sidebar can be toggled via a burger button and closes on navigation.
  • Simplified the top banner layout (title, logo, location only).
  • Updated spec.cy.js test code to match the new layout structure:
    • Adjusted selectors and actions to open the sidebar before interacting with nav links or controls.
    • Ensured UI controls are still functional in their new locations.

…antine-components

- updated dash and dash-mantine-components
- moved the navigation and document, global, and IP to the sidebar
- modified the spec.cy.js
- updated the tabs.css and layout.css
@coderabbitai
Copy link

coderabbitai bot commented Sep 10, 2025

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@LeoLuosifen LeoLuosifen marked this pull request as ready for review September 10, 2025 04:59
Copy link
Contributor

@FedericoTartarini FedericoTartarini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a few comments, I did not review the whole code. In summary do not use CSS unless 100% needed, it is generally not needed since mantine components are well designed. Also do not simply replace Div with Box, but instead check if the Div is needed at all, if not just remove it. I am sure that more than 50% of them can be removed by using the right mantine components like Stack, Group, Grid, ...

@Sallie00
Copy link
Contributor

After thorough investigation, we have traced the issue back to the utils.py file.

We discovered that replacing the html.A component with its Dash Mantine equivalent (e.g., dmc.Anchor) causes unexpected redirects to the home page, especially during Cypress tests. This prevents the /summary page from loading properly and makes it impossible to verify elements on that page.

To ensure stability, we have kept this single use of html.A unchanged. All other HTML elements have been successfully migrated to Dash Mantine Components.

Please let us know how you’d prefer to proceed with this specific case.
@FedericoTartarini

@FedericoTartarini
Copy link
Contributor

@Sallie00 I am happy with your decision. please keep those html.A components when necessary and otherwise breaking the code. I like this since we can still use the old test framework. Please tag me here once the pull request is complete so I will review it.

@Sallie00
Copy link
Contributor

Hi @FedericoTartarini , The pull request is now ready for your review.

Copy link
Contributor

@FedericoTartarini FedericoTartarini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sallie00 I really like the work; however, I noticed a major issue. The navbar is closed by default, and it will be very confusing for current and future users to find the tabs. The navbar, to work well:

  1. needs to be open by default
  2. it should not be going over the page but shift the page as I mentioned before, see this website as an example https://fly-log.netlify.app/
  3. this is very important also because it is a bit annoying to have to open the navbar each time the user wants to change the page.
  4. the logo in the title of the navbar is not showing
  5. reduce sligthly the spacing between the links in the navbar.

code review

Very good work I'm very happy with the progress. However, I think they code can be further simplified. For example, I think you have too many nested components. Please review all my comments carefully. I am a little bit worried because sometimes you change the argument of the input or of the output in the callback. Please make sure that this is what you want to do. Please also try to use as much as possible the default component as provided by Mantine. Using the default will ensure that the website is consistent and beautiful.

@LeoLuosifen
Copy link
Contributor Author

@FedericoTartarini
Hi Federico, I've restructured two versions of the layout of the Clima. Both are used with AppShell to manage.

Version 1

version_1_1
version_1_2

The first version uses the Full AppShell Layout https://py.cafe/dash.mantine.components/AppShell-with-all-elements, divided into header, navbar, main, and footer sections. The main section is scrollable, though this may not be readily apparent in my screenshot. Simultaneously, clicking the burger button controls the display and concealment of the sidebar, functioning identically to the demonstration in Dash Mantine. By default, it remains expanded.

Version 2

version_2_1
version_2_2

The secondary version uses Collapsible AppShell Layout https://py.cafe/dash.mantine.components/Collapsible-navbar-on-both-desktop-and-moble, divided into header, navbar, and main sections. The main section includes the original layout's main and footer elements and is also scrollable. The function of the burger button remains unchanged from Version 1.

Which version do you prefer?

@FedericoTartarini
Copy link
Contributor

@LeoLuosifen I am not fully understanding what the difference is between v1 and v2. Please explain the key differences and how this will affect the user experience of Clima.

@LeoLuosifen
Copy link
Contributor Author

@FedericoTartarini The entire page appears somewhat small in v1, as my computer screen is rather compact. While it might look better on a larger display, this may not suit all users. The v2 more closely looks like the effect shown in the link you provided https://fly-log.netlify.app/.

@FedericoTartarini
Copy link
Contributor

I want to recreate the same effect as in https://fly-log.netlify.app/. please also note that the navbar width can be slighlty reduced since it does not need to be so wide. If version 2 is similar to https://fly-log.netlify.app/ then we can implement version 2

- optimized the original simple dmc components
- removed the element ids are no longer needed
- removed the unnecessary css style in css files
- removed page_icon.py, moved to layout.py and renamed class name to NavbarIcons
- updated spec.cy.js
- reformatted the code
@LeoLuosifen
Copy link
Contributor Author

@FedericoTartarini
Hi Federico, we've removed the unnecessary spacing and made the default sidebar of the pages menu open, also ensured the sidebar width is consistent. Could you kindly review it and let us know if there's anything else that could be improved?
sidebar_1
sidebar_2

Removed unnecessary Stack component to streamline the layout
and improve readability. Also optimized spacing in the layout
by removing redundant CSS properties.
Replace Box with Stack for improved layout structure and
remove unnecessary style properties to simplify the code.
…raphs

Consolidate container and stack components for improved readability and
maintainability. Adjust grid column spans for responsive design.
Clean up the layout by eliminating redundant margin and padding properties
in the title_with_tooltip function to streamline the component's styling.
Updated the layout function to utilize the Stack component instead of
Container, improving the structure and alignment of child elements.
Refactor the layout function for tab four by removing the old definition
and ensuring the layout is consistently defined using the Stack component.
Replace dcc.Loading with dmc.Center for improved layout consistency.
Remove unused CSS styles related to tabs for cleaner codebase.
Refactor the layout function to use a Stack component instead of Box,
improving the structure and consistency of the layout in tab six.
Eliminate fixed y-axis range to allow for dynamic scaling based on data.
Clean up the code by removing hardcoded width styles from various dropdowns
to improve layout consistency and maintainability.
Reorganize the layout code in section one for better readability and maintainability.
This includes consistent indentation and formatting adjustments.
@FedericoTartarini
Copy link
Contributor

Very good work, I really like what you have done.

Please all carefully review all my changes. This will be an opportunity for you to learn and see how css could have been removed and the code could have been much further simplified. I did not implemented all the changes I wanted but as you can see we could much furhter improve the code and avoiding nesting components. I will need to stop here for today and I will accept the pull request for now, but note that we can improve it much further.

@FedericoTartarini FedericoTartarini merged commit 60214b6 into CenterForTheBuiltEnvironment:development Sep 26, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants